Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

doc: clarify IncomingMessage is a new Readable "forked" from the socket #36641

Merged
merged 1 commit into from
Dec 30, 2020
Merged

doc: clarify IncomingMessage is a new Readable "forked" from the socket #36641

merged 1 commit into from
Dec 30, 2020

Conversation

dr-js
Copy link
Contributor

@dr-js dr-js commented Dec 27, 2020 •
edited
Loading

Refs: #36617

Added more description on the difference between the IncomingMessage and the underlying message.socket.

Added a separate commit to mark the change of v15.5.0 and deprecate message.aborted per #36617 (comment). (moved to #36670)

Checklist

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem. labels Dec 27, 2020
@dr-js dr-js mentioned this pull request Dec 27, 2020
Closed
targos
targos reviewed Dec 27, 2020
View reviewed changes
doc/api/http.md Outdated Show resolved Hide resolved
@aduh95 aduh95 added notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version. labels Dec 27, 2020
RaisinTen
RaisinTen reviewed Dec 27, 2020
View reviewed changes
doc/api/http.md Outdated Show resolved Hide resolved
aduh95
aduh95 requested changes Dec 27, 2020
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should add the deprecation info to doc/api/deprecations.md as well.

@targos
Copy link
Member

targos commented Dec 27, 2020

Do we have a policy to treat doc-only deprecations as semver-minor?

@aduh95 aduh95 removed the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 27, 2020
@dr-js dr-js force-pushed the patch-1 branch 2 times, most recently from 76a9da1 to 8bcc099 Compare December 28, 2020 00:51
aduh95
aduh95 reviewed Dec 28, 2020
View reviewed changes
doc/api/deprecations.md Outdated Show resolved Hide resolved
@dr-js
Copy link
Contributor Author

dr-js commented Dec 28, 2020

@aduh95 I wrote some test to compare current message.aborted behavior with message.destroyed at test-nodejs-message-aborted.js, and here is what I think:

Only directly call .abort() will emit the 'abort' event and set .aborted
to true, the .destroyed attribute will be set to true at the same time,
and the Stream 'error' and 'close' event will follow. Calling the Stream
.destroy() will emit only the 'close' event, and may not set the .aborted
to true.

So the abort related value and event is only useful for detecting .abort()
calls. For closing a request early, call the Stream .destroy() and listen for
'close' and 'error' should have the same effect and wider code support.

I'll add the second paragraph as the explanation.

Do we doc-deprecate event?
As currently request.abort() is marked, but the 'abort' event is not, so is the request.aborted kept.

Also it's better if @ronag could review the added explanation.

@ronag
Copy link
Member

ronag commented Dec 28, 2020 •
edited
Loading

I think deprecation is something for a separate PR. I would personally be happy to doc deprecate:

  • 'abort' event.
  • 'aborted' event.
  • .aborted property.
  • .abort() function.

@dr-js
Copy link
Contributor Author

dr-js commented Dec 29, 2020

@ronag then I'll split deprecation related commit to a separate PR and link back

@dr-js dr-js mentioned this pull request Dec 29, 2020
Closed
2 tasks
dr-js added a commit to dr-js/node that referenced this pull request Dec 29, 2020
@dr-js @targos
doc: deprecate (doc-only) http abort related
80ee6ee 
Refs: nodejs#36641
Refs: nodejs#36617 (comment)

Documentation-only deprecate `.aborted` property and `'abort'`, `'aborted'` event in `http`, and suggest using the corresponding Stream API instead.

Co-authored-by: Michaël Zasso <targos@protonmail.com>
@dr-js dr-js requested a review from aduh95 December 30, 2020 11:53
@aduh95 aduh95 removed the notable-change PRs with changes that should be highlighted in changelogs. label Dec 30, 2020
@dr-js @aduh95
doc: document http.IncomingMessage behaviour change
a5b43e9 
Add to the history table that the `destroyed` value returns `true` after
the incoming data is consumed.

Refs: nodejs#36617
Refs: nodejs#33035

PR-URL: nodejs#36641
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
@aduh95 aduh95 self-assigned this Dec 30, 2020
@aduh95
Copy link
Contributor

aduh95 commented Dec 30, 2020

Landed in a5b43e9

@aduh95 aduh95 merged commit a5b43e9 into nodejs:master Dec 30, 2020
@dr-js dr-js deleted the patch-1 branch December 30, 2020 15:39
danielleadams pushed a commit that referenced this pull request Jan 12, 2021
@dr-js @danielleadams
doc: document http.IncomingMessage behaviour change
651e7d2 
Add to the history table that the `destroyed` value returns `true` after
the incoming data is consumed.

Refs: #36617
Refs: #33035

PR-URL: #36641
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
@danielleadams danielleadams mentioned this pull request Jan 12, 2021
Merged
dr-js added a commit to dr-js/node that referenced this pull request Jan 19, 2021
@dr-js @targos
@Trott @ronag @aduh95
doc: deprecate (doc-only) http abort related
bc927d6 
Refs: nodejs#36641
Refs: nodejs#36617 (comment)

Documentation-only deprecate `.aborted` property and `'abort'`, `'aborted'` event in `http`, and suggest using the corresponding Stream API instead.

Co-authored-by: Michaël Zasso <targos@protonmail.com>
Co-authored-by: Rich Trott <rtrott@gmail.com>
Co-authored-by: Robert Nagy <ronagy@icloud.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
dr-js added a commit to dr-js/node that referenced this pull request Feb 3, 2021
@dr-js @targos
@Trott @ronag @aduh95
doc: deprecate (doc-only) http abort related
2eb751c 
Refs: nodejs#36641
Refs: nodejs#36617 (comment)

Documentation-only deprecate `.aborted` property and `'abort'`, `'aborted'` event in `http`, and suggest using the corresponding Stream API instead.

Co-authored-by: Michaël Zasso <targos@protonmail.com>
Co-authored-by: Rich Trott <rtrott@gmail.com>
Co-authored-by: Robert Nagy <ronagy@icloud.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
dr-js added a commit to dr-js/node that referenced this pull request Mar 4, 2021
@dr-js @targos
@Trott @ronag @aduh95
doc: deprecate (doc-only) http abort related
9fec6fd 
Refs: nodejs#36641
Refs: nodejs#36617 (comment)

Documentation-only deprecate `.aborted` property and `'abort'`, `'aborted'` event in `http`, and suggest using the corresponding Stream API instead.

Co-authored-by: Michaël Zasso <targos@protonmail.com>
Co-authored-by: Rich Trott <rtrott@gmail.com>
Co-authored-by: Robert Nagy <ronagy@icloud.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
dr-js added a commit to dr-js/node that referenced this pull request Sep 16, 2021
@dr-js @targos
@Trott @ronag @aduh95
doc: deprecate (doc-only) http abort related
bc1f2a9 
Refs: nodejs#36641
Refs: nodejs#36617 (comment)

Documentation-only deprecate `.aborted` property and `'abort'`, `'aborted'` event in `http`, and suggest using the corresponding Stream API instead.

Co-authored-by: Michaël Zasso <targos@protonmail.com>
Co-authored-by: Rich Trott <rtrott@gmail.com>
Co-authored-by: Robert Nagy <ronagy@icloud.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
dr-js added a commit to dr-js/node that referenced this pull request Sep 16, 2021
@dr-js @targos
@Trott @ronag @aduh95
doc: deprecate (doc-only) http abort related
3e204fd 
Refs: nodejs#36641
Refs: nodejs#36617 (comment)

Documentation-only deprecate `.aborted` property and `'abort'`,
`'aborted'` event in `http`, and suggest using the corresponding
Stream API instead.

Co-authored-by: Michaël Zasso <targos@protonmail.com>
Co-authored-by: Rich Trott <rtrott@gmail.com>
Co-authored-by: Robert Nagy <ronagy@icloud.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
dr-js added a commit to dr-js/node that referenced this pull request Sep 16, 2021
@dr-js @targos
@Trott @ronag @aduh95
doc: deprecate (doc-only) http abort related
d9fb8ae 
Refs: nodejs#36641
Refs: nodejs#36617 (comment)

Documentation-only deprecate `.aborted` property and `'abort'`,
`'aborted'` event in `http`, and suggest using the corresponding
Stream API instead.

Co-authored-by: Michaël Zasso <targos@protonmail.com>
Co-authored-by: Rich Trott <rtrott@gmail.com>
Co-authored-by: Robert Nagy <ronagy@icloud.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
mcollina pushed a commit that referenced this pull request Sep 27, 2021
@dr-js @mcollina
doc: deprecate (doc-only) http abort related
4b030d0 
Refs: #36641
Refs: #36617 (comment)

Documentation-only deprecate `.aborted` property and `'abort'`,
`'aborted'` event in `http`, and suggest using the corresponding
Stream API instead.

Co-authored-by: Michaël Zasso <targos@protonmail.com>
Co-authored-by: Rich Trott <rtrott@gmail.com>
Co-authored-by: Robert Nagy <ronagy@icloud.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: #36670
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
dr-js added a commit to dr-js/node that referenced this pull request Oct 5, 2021
@dr-js @targos
@Trott @ronag @aduh95
doc: deprecate (doc-only) http abort related
9a035e1 
Refs: nodejs#36641
Refs: nodejs#36617 (comment)

Documentation-only deprecate `.aborted` property and `'abort'`,
`'aborted'` event in `http`, and suggest using the corresponding
Stream API instead.

Co-authored-by: Michaël Zasso <targos@protonmail.com>
Co-authored-by: Rich Trott <rtrott@gmail.com>
Co-authored-by: Robert Nagy <ronagy@icloud.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
dr-js added a commit to dr-js/node that referenced this pull request Oct 5, 2021
@dr-js
doc: deprecate (doc-only) http abort related
8c76fa5 
Refs: nodejs#36641
Refs: nodejs#36617 (comment)

Documentation-only deprecate `.aborted` property and `'abort'`,
`'aborted'` event in `http`, and suggest using the corresponding
Stream API instead.

Co-authored-by: Michaël Zasso <targos@protonmail.com>
Co-authored-by: Rich Trott <rtrott@gmail.com>
Co-authored-by: Robert Nagy <ronagy@icloud.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: nodejs#36670
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this pull request Oct 9, 2021
@dr-js @targos
doc: deprecate (doc-only) http abort related
4da1318 
Refs: #36641
Refs: #36617 (comment)

Documentation-only deprecate `.aborted` property and `'abort'`,
`'aborted'` event in `http`, and suggest using the corresponding
Stream API instead.

Co-authored-by: Michaël Zasso <targos@protonmail.com>
Co-authored-by: Rich Trott <rtrott@gmail.com>
Co-authored-by: Robert Nagy <ronagy@icloud.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: #36670
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
danielleadams pushed a commit that referenced this pull request Oct 12, 2021
@dr-js @danielleadams
doc: deprecate (doc-only) http abort related
d1d9f2d 
Refs: #36641
Refs: #36617 (comment)

Documentation-only deprecate `.aborted` property and `'abort'`,
`'aborted'` event in `http`, and suggest using the corresponding
Stream API instead.

Co-authored-by: Michaël Zasso <targos@protonmail.com>
Co-authored-by: Rich Trott <rtrott@gmail.com>
Co-authored-by: Robert Nagy <ronagy@icloud.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: #36670
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaisinTen RaisinTen RaisinTen left review comments

@targos targos targos left review comments

@aduh95 aduh95 aduh95 approved these changes

@ronag ronag ronag approved these changes

Assignees

@aduh95 aduh95

Labels
doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dr-js @targos @ronag @aduh95 @RaisinTen @nodejs-github-bot